Can you go over why we think (sometimes) parsing the environment variables as opposed to always passing them as is in a string is the the best choice?

Until yesterday I thought that otherwise there would no way to write something like:

learning_rate: ${env:LR}

and have learning_rate be a float.

That being said, I just realized that {Integer,Float,Boolean}Node actually convert from string, so maybe this would work with structured configs?
(although I'm not entirely sure why this is allowed in the first place, my first instinct would be that assigning a string to an int/float/bool node should raise an exception)

Well, you can check and see for yourself :).
I think the answer is no, because the result if the interpolation is returned directly and is not assigned to the config.

Converting on assignment was introduced really early and may have been a mistake.
The motivation is to support converting during merge: in particular merging from cli, where everything is a string.

We should consider deprecating support for assignments that mypy would frowns upon if it knew the type.

@dataclass
class Foo:
  x: int = 10

cfg: Foo = OmegaConf.structured(Foo)
cfg.x = "20"  # mypy would bitch here even though it's legal in OmegaConf.

We should consider deprecating support for assignments that mypy would frowns upon if it knew the type.

I created #459 to keep track of this

Until yesterday I thought that otherwise there would no way to write something like:

learning_rate: ${env:LR}

and have learning_rate be a float.

Issue 488 is a recent report about this.
I think we should consider validating the return value of custom interpolations against the specified type.
This is a breaking change, so switching to new resolvers is a good opportunity (with a potential opt-out flag - maybe).
As a POC, I gave it a shot in #506 (based on the current interpolation logic). It adds validation for nodes that are annotated (ValueNodes only, and only if they are not AnyNode).

Just a note that this actually doesn't work, I'll look into fixing it. Just to be sure, do we want the following to work:

@dataclass
class Cfg:
    s: str = "7"
    i: int = II("s")

?
(the question is, since s can be cast to int, is it ok to do it implicitly here as well?)

As discussed, I reverted the changes that were (attempting at) addressing #488. I will do that in a follow-up PR instead, to avoid adding too much stuff here (since it's already way too big).

As a result, I also believe that it's best to also revisit the behavior of the env resolver in another follow-up PR, so that when env is changed to only return strings, then at least typed nodes keep working as expected (even without a new decode resolver).

@omry if you're ok with that plan then this conversation may be resolved.

resolving.

Just to clarify:
There are multiple things here that should probably be changed.

1. Support for type annotation for resolvers (with runtime validation).
2. Env resolver should no longer parse input environment variables.
3. Potentially new built in resolver oc.decode() that can parse an input with the grammar parser.

488 is about 1.
2 and 3 are related.

I agree, they will be addressed in that order in future PRs

I was expecting to see some parser specific unit tests (testing the parser directly without going through the higher layers).
Did I miss it?

No, I didn't realize it was possible until later, at which point I had already written all tests this way.
Do you want me to refactor them?

I missed this one.
Yes - I think having lower level tests will make the tests more robust and easier to work with.
Once you have lower level tests, this entire function can probably be removed.

I refactored these tests in b520815

Essentially I split the big list into 3:

• Stuff that can be parsed without interpolations (to test the basic constructs of the grammar, like int, float, str, dict, etc.). Those can be parsed without using any config object.
• Expressions containing interpolations for the parser rule singleElement (this is the biggest chunk)
• Finally, expressions for the highest-level parser rule configValue

In the end it still looks pretty similar overall, but at least it's faster and avoids using dummy resolvers everywhere just to test the correct parser rule. Let me know if that looks ok to you.

NB: in a first attempt I tried to go even lower in the rules (e.g., using the dictContainer parser rule for dicts), but the problem is that without an EOF in the parser rule, ANTLR fails to detect some errors (ex: {0: 1}} is not supposed to be a valid dict). That's why I stuck to the singleElement and configValue rules (that contain the EOF).

Until yesterday I thought that otherwise there would no way to write something like:

learning_rate: ${env:LR}

and have learning_rate be a float.

Issue 488 is a recent report about this.
I think we should consider validating the return value of custom interpolations against the specified type.
This is a breaking change, so switching to new resolvers is a good opportunity (with a potential opt-out flag - maybe).
As a POC, I gave it a shot in #506 (based on the current interpolation logic). It adds validation for nodes that are annotated (ValueNodes only, and only if they are not AnyNode).